Skip to content

Conversation

kcieplak
Copy link
Contributor

@kcieplak kcieplak commented Oct 3, 2025

Fix a crash when unwrapping an optional 'mainModule' of a module. In the case where a plugin has a dependency on a 'binaryTarget' there is no main module.

This only presents itself when you have defined the binaryTarget as both a product and target.

  • Update method that determines plugin dependencies to handle binaryTarget types explicitly.
  • Add a PIF generation test that captures this behaviour.

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 3, 2025

@swift-ci test

@pmattos pmattos self-requested a review October 3, 2025 17:43
}
}

@Test func pluginWithBinaryTargetDependency() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Great to see more PIF tests in OSS SPM ☺️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @pmattos. We should expand the PIF test coverage.

Although this is a great test, it's a large test (ie: end-to-end). Can we investigate writing tests lower in the test pyramid, possibly testing productRepresentingDependencyOfBuildPlugin in isolation?

Fix a crash when unwrapping an optional 'mainModule' of a module.
In the case where a plugin has a dependency on a 'binaryTarget'
there is no main module.

This only presents itself when you have defined the binaryTarget as
both a product and target.

* Update method that determines plugin dependencies to handle
  binaryTarget types explicitly.
* Add a PIF generation test that captures this behavior.
@kcieplak kcieplak force-pushed the topics/fix-gh-9067 branch from fec6d4e to 921daeb Compare October 3, 2025 17:57
@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 3, 2025

Fixes #9067

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 3, 2025

@swift-ci test

@pmattos
Copy link
Contributor

pmattos commented Oct 3, 2025

praise: Matthew just confirmed this fixes rdar://159087121 in Xcode too :)

@pmattos
Copy link
Contributor

pmattos commented Oct 3, 2025

I've a PR up to validate this patch within Xcode... will report back the results :)

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 3, 2025

@swift-ci test

ci.swift.org was 504 for a while

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 3, 2025

@swift-ci test

2 similar comments
@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 3, 2025

@swift-ci test

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 4, 2025

@swift-ci test

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 4, 2025

@swift-ci test windows

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 4, 2025

@swift-ci test windows self hosted

#expect(executableTarget.common.name == "MyPluginExe")

// Verify no errors were emitted during PIF generation
let errors = observabilitySystem.diagnostics.filter { $0.severity == .error }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: do we expect warnings to occur? If no, should we also ensure no warnings exist? if the answer is "we don't care about warning", is there benefits adding a comment indicating so?

@bkhouri
Copy link
Contributor

bkhouri commented Oct 6, 2025

Looks a Swift Testing test was not terminated correctly

08:37:52  [1464/1464] Testing SourceControlTests.RepositoryManagerTests/testCancel
14:46:24  Cancelling nested steps due to timeout
14:46:24  Sending interrupt signal to process
14:46:44  After 20s process did not stop

Re-triggering the build

@swift-ci test self hosted windows

@kcieplak kcieplak requested a review from pmattos October 6, 2025 16:39
@pmattos
Copy link
Contributor

pmattos commented Oct 6, 2025

I've a PR up to validate this patch within Xcode... will report back the results :)

All good in Xcode too 🚀

Based on Review.
* Remove unwrapping and use the guarded mainModule variable.
@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 6, 2025

@swift-ci test

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 6, 2025

@swift-ci test windows

@kcieplak
Copy link
Contributor Author

kcieplak commented Oct 6, 2025

@swift-ci test macOS

@kcieplak kcieplak merged commit e66f301 into swiftlang:main Oct 7, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants